feat(invitations): complete external ID bulk invite flow and redesign result dialog#8406
feat(invitations): complete external ID bulk invite flow and redesign result dialog#8406LWS49 wants to merge 2 commits into
Conversation
c0dcc05 to
e892fd9
Compare
d6e1808 to
b75a804
Compare
a037a06 to
023720d
Compare
9c8e986 to
007edda
Compare
0ef7442 to
62635b4
Compare
62635b4 to
ba302dd
Compare
6226857 to
9c27231
Compare
d83d859 to
4087560
Compare
06af4fa to
ae12c5e
Compare
3498902 to
9c390c4
Compare
309b762 to
85a592d
Compare
| import tableTranslations from 'lib/translations/table'; | ||
|
|
||
| const translations = defineMessages({ | ||
| yes: { |
There was a problem hiding this comment.
Can we use lib.translations.yes here? (and for no)
There was a problem hiding this comment.
Fixed in ExistingTable, FailedTable, and PrimaryTable — all three now import and use libTranslations.yes / libTranslations.no from lib/translations.
| defaultMessage: | ||
| 'Failed to send invitation email, please try again - if failures persist, contact us for assistance', | ||
| }, | ||
| yes: { |
There was a problem hiding this comment.
same here, let's use lib.translations.yes to minimize unneccesary duplication
There was a problem hiding this comment.
Fixed — same as above, using libTranslations now.
| query = Course::UserInvitation.unconfirmed.where(course_id: course_id, external_id: external_id) | ||
| query = query.where.not(id: id) if is_a?(Course::UserInvitation) | ||
| query.exists? | ||
| scope = Course::UserInvitation.unconfirmed.where(course_id: course_id, external_id: external_id) |
There was a problem hiding this comment.
I think this change is unneccessary, query was a perfectly fine variable name before. I feel semantic-only changes only add noise to the blame history.
| def parse_file_row(row) | ||
| return nil if row[1].blank? | ||
|
|
||
| if !@current_course.show_personalized_timeline_features? && row.length > 5 |
There was a problem hiding this comment.
Part of me feels like there should be an analogous check for when personalized timelines is enabled.
There was a problem hiding this comment.
On reflection, the analogous check (timelines enabled, fewer than 6 columns) isn't meaningful to add. A row with as few as 2 columns is valid - role, phantom, timeline algorithm, and external ID all have sensible defaults, so a short row is not an error worth raising. A "too many columns" check in the other direction similarly serves no purpose here.
More broadly, this check is a temporary gap-stopper: #8427 replaces it with proper header-based validation, which handles both directions correctly. Given its short lifespan, keeping the current check narrow feels appropriate.
There was a problem hiding this comment.
Pull request overview
Completes the bulk-invite “external ID” upsert flow in the backend service and updates the frontend invitation result UX to accurately report failures/updates (including Personalized Timeline display) while simplifying result categorization.
Changes:
- Extend
Course::UserInvitationServiceto support external ID upserts on existing invitations/course users, track update metadata, and replaceduplicate_userswithfailed_users. - Redesign the invitation result dialog into primary/existing/failed tables, add updated-row highlighting + tooltips, and show Personalized Timeline column based on
showPersonalizedTimelineFeatures. - Add guards + messaging for CSV template mismatch (6-column upload when timelines are disabled), and extend tests/specs accordingly.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/services/course/user_invitation_service_spec.rb | Updates/extends service specs for failed users, ext_id upsert, CSV mismatch, and timeline assignment behavior. |
| spec/models/user_spec.rb | Adds regression spec for blank invitation external_id coercing to nil on built CourseUser. |
| spec/models/course_user_spec.rb | Adds regression coverage for external_id uniqueness vs pending invitations (including update/id edge case). |
| spec/controllers/course/user_invitations_controller_spec.rb | Ensures isRetryable is included in existingInvitations result payload. |
| config/locales/zh/errors.yml | Adds timeline template mismatch error message (Chinese). |
| config/locales/zh/csv.yml | Improves Chinese CSV header translations (“Email”/role wording). |
| config/locales/ko/errors.yml | Adds timeline template mismatch error message (Korean). |
| config/locales/en/errors.yml | Adds timeline template mismatch error message (English). |
| client/locales/zh.json | Updates invitation-result + failed-table translations (Chinese). |
| client/locales/ko.json | Updates invitation-result + failed-table translations (Korean). |
| client/locales/en.json | Updates invitation-result + failed-table translations (English). |
| client/app/types/course/userInvitations.ts | Renames/reshapes result payload types (failedUsers, updated items, reasons). |
| client/app/types/course/courseUsers.ts | Adds showPersonalizedTimelineFeatures to shared manage-users data type. |
| client/app/bundles/course/user-invitations/store.ts | Seeds showPersonalizedTimelineFeatures into invitations bundle initial state. |
| client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/index.tsx | Updates external ID help text to reflect overwrite/upsert semantics. |
| client/app/bundles/course/user-invitations/pages/InvitationsIndex/test/index.test.tsx | Adds frontend regression tests for timeline column gating + correct store selector usage. |
| client/app/bundles/course/user-invitations/components/tables/UserInvitationsTable.tsx | Fixes selector import and gates timeline column on course feature flag (not permission). |
| client/app/bundles/course/user-invitations/components/tables/InvitationResultUsersTable.tsx | Removes legacy “users result” table (replaced by new tables). |
| client/app/bundles/course/user-invitations/components/tables/InvitationResultPrimaryTable.tsx | New table for successful rows (new invitations/enrollments), with optional timeline column + CSV download. |
| client/app/bundles/course/user-invitations/components/tables/InvitationResultInvitationsTable.tsx | Removes legacy “invitations result” table (replaced by new tables). |
| client/app/bundles/course/user-invitations/components/tables/InvitationResultFailedTable.tsx | New failed rows table (reason mapping + red highlighting for failed_to_send). |
| client/app/bundles/course/user-invitations/components/tables/InvitationResultExistingTable.tsx | New existing/updated rows table (update highlighting + “Previously” tooltip). |
| client/app/bundles/course/user-invitations/components/tables/test/InvitationResultPrimaryTable.test.tsx | Unit tests for primary table columns, timeline gating, and ext_id column behavior. |
| client/app/bundles/course/user-invitations/components/tables/test/InvitationResultFailedTable.test.tsx | Unit tests for failed table reasons, highlighting, and timeline gating. |
| client/app/bundles/course/user-invitations/components/tables/test/InvitationResultExistingTable.test.tsx | Unit tests for existing table ordering, tooltip behavior, highlighting, and timeline gating. |
| client/app/bundles/course/user-invitations/components/misc/InvitationResultDialog.tsx | Rebuilds dialog summary + sections, consolidates failures, and threads timeline feature flag from store. |
| client/app/bundles/course/user-invitations/components/misc/test/InvitationResultDialog.test.tsx | Integration tests for section routing, counts, ordering, and timeline column visibility via store. |
| client/app/bundles/course/enrol-requests/store.ts | Seeds showPersonalizedTimelineFeatures into enrol-requests bundle initial state. |
| app/views/course/user_invitations/index.json.jbuilder | Exposes showPersonalizedTimelineFeatures in shared manage-users data. |
| app/views/course/user_invitations/_invitation_result_data.json.jbuilder | Serializes timeline algorithm, retryability, failed users, and updated items into result payload. |
| app/services/course/user_invitation_service.rb | Expands service return tuple, saves updated records before inserts, and renames duplicate→failed tracking. |
| app/services/course/statistics/assessments_score_summary_download_service.rb | Uses .presence for external_id CSV column output. |
| app/services/concerns/course/user_invitation_service/process_invitation_concern.rb | Implements ext_id upsert for existing invitations/course users + timeline_algorithm direct-enrollment fix. |
| app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb | Detects 6-column CSV mismatch when timelines are off and normalizes duplicate rows into failed_users. |
| app/models/user.rb | Coerces invitation external_id via .presence when building CourseUser. |
| app/models/concerns/course/unique_external_id_concern.rb | Refactors uniqueness checks to avoid incorrect invitation exclusion logic. |
| app/controllers/course/user_invitations_controller.rb | Updates result renderer signature to include failed + updated collections. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| json.failedUsers failed_users.each.with_index do |failed_user, index| | ||
| json.id index | ||
| json.name duplicate_user[:name] | ||
| json.email duplicate_user[:email] | ||
| json.externalId duplicate_user[:external_id] | ||
| json.role duplicate_user[:role] | ||
| json.phantom duplicate_user[:phantom] | ||
| json.reason duplicate_user[:reason] | ||
| json.name failed_user[:name] |
There was a problem hiding this comment.
Updated — entries with a real DB id (e.g. AR-backed records pushed with external_id_taken) now keep their own id; hash-only entries fall back to -(index + 1).
| previouslyLabel: { | ||
| id: 'course.userInvitations.InvitationResultExistingTable.previouslyLabel', | ||
| defaultMessage: 'Previously: {value}', | ||
| }, |
There was a problem hiding this comment.
Fixed — added course.userInvitations.InvitationResultExistingTable.previouslyLabel to en.json, zh.json, and ko.json.
| "course.userInvitations.InvitationResultFailedTable.externalIdTakenEnrolled": { | ||
| "defaultMessage": "已是课程成员 — 外部编号未能应用(已分配给另一名成员" | ||
| }, |
There was a problem hiding this comment.
Fixed — added the missing closing ).
| invalid_email: '%{email} could not be processed: invalid email: %{message}.' | ||
| duplicate_external_id: '%{email} could not be processed: external ID "%{external_id}" is already taken.' | ||
| other_error: '%{email} could not be processed: %{message}.' | ||
| timeline_template_mismatch: 'more than 5 columns detected. This course does not use Personalized Timelines. Please re-upload using the 5-column template (Name, Email, Role, Phantom, External ID).' |
| # @param [Array<Hash>|File|TempFile] users Invites the given users. | ||
| # @return [Array<Integer>|nil] An array containing the the size of new_invitations, existing_invitations, | ||
| # new_course_users and existing_course_users, duplicate_users respectively if success. nil when fail. | ||
| # @return [Array<Integer>|nil] An array containing the size of new_invitations, existing_invitations, | ||
| # new_course_users, existing_course_users, failed_users, updated_invitations, updated_course_users | ||
| # respectively if success. nil when fail. |
There was a problem hiding this comment.
Fixed — docstring now says the method returns the seven arrays, not their sizes.
| "course.userInvitations.InvitationResultDialog.summaryFailed": { | ||
| "defaultMessage": "{count}건이 실패했습니다." | ||
| }, | ||
| "course.userInvitations.InvitationResultDialog.updatedSubtitle": { | ||
| "defaultMessage": "{count}개 업데이트됨 · 먼저 표시" | ||
| }, |
There was a problem hiding this comment.
Fixed — added summary and failedRowsSubtitle to ko.json.
| "course.userInvitations.InvitationResultDialog.summaryFailed": { | ||
| "defaultMessage": "{count} 个失败。" | ||
| }, | ||
| "course.userInvitations.InvitationResultUsersTable.duplicateExternalIdInFile": { | ||
| "course.userInvitations.InvitationResultDialog.updatedSubtitle": { | ||
| "defaultMessage": "{count} 条已更新 · 优先显示" | ||
| }, |
There was a problem hiding this comment.
Fixed — added summary and failedRowsSubtitle to zh.json.
23c59ca to
559264e
Compare
… dialog - add ext_id to CourseUser and UserInvitation with unique-per-course index - accept ext_id in bulk CSV and individual invite form; upsert for existing records (enrolled users and pending invitations) - conflicts (duplicate email or ext_id) surface in Failed with reasons
546aed9 to
b379882
Compare
8ed6794 to
db90f87
Compare
…led user to fall under Failed instead of Existing
db90f87 to
b761414
Compare
Part 1: External ID in the invite flow
Summary
Completes the external ID bulk invite flow started in the previous PR. The CSV upload now acts as a full upsert for external IDs on existing records: a nil ext_id is filled in, a non-nil ext_id that differs from the CSV value is overwritten (not rejected), and a blank CSV cell preserves the existing value. The
external_id_conflicterror condition is removed - the only remaining hard rejection for ext_id isexternal_id_taken(the CSV value is already held by a different course member). Updated rows fold into the Existing Invitations and Existing Course Users sections with a bold ext_id badge and old-value tooltip; the separate "External IDs Updated" section is removed. Error messages in the controller are now per-error-type rather than a single sentence, enabling the result dialog to attribute each failure reason precisely. Theexternal_idfield is carried through toCourseUseron invitation acceptance. The score summary CSV download gains an opt-in ext_id column, shown only when at least one student has a non-blank ext_id.Design decisions
external_id_taken(ID collision across different people), which is still a hard error. Flagging a staff member correcting a previous ext_id entry as a conflict imposes workflow cost with no safety gain. Nil-to-value and non-nil-to-different-value are now treated symmetrically.freed-idtonew-idand Bob claimsfreed-idin the same batch, Alice's update must persist first so Bob's insert doesn't hit the unique constraint.external_idcoerced via.presencewhen building CourseUser from invitation - the partial unique index oncourse_users(course_id, external_id)only covers non-NULL values (WHERE external_id IS NOT NULL). Storing an empty string""would subject it to the uniqueness constraint, causing duplicate-key errors when multiple invitees have no external ID set..presenceconverts""→nil, keeping those rows outside the partial index scope.show_personalized_timeline_features?is false, the parser readsrow[4]asexternal_id. A 6-column file (the timeline template) would silently store the timeline string (e.g."otot") as the external ID. Column count detection (row.length > 5) is used as the signal: value-matching against known algorithm names was rejected because those strings are valid external IDs. The upload is aborted before any row is processed and a descriptive toast tells staff to use the 5-column template.partition_unique_users(CSV dedup) ran beforeaugment_user_objects(DB lookup), so the dedup had no way to know that a row belonged to an existing account. An enrolled user re-uploaded with their current external ID alongside a new user holding the same ID was incorrectly rejected asduplicate_external_id_in_fileat the CSV phase, before the DB-aware process phase had a chance to recognise them as already enrolled. The fix moves the DB lookup first;partition_unique_usersnow receives the resolvedexisting_account_emailsset and exempts those rows from external-ID dedup, leaving them for the process phase which handles them correctly.Service behavior: master vs this PR
Ext_id is checked at four points in the service pipeline.
@taken_external_idsis pre-loaded from bothCourseUserand unconfirmedCourse::UserInvitationext_ids, so a conflict with either type anchors the same rejection.@taken_external_ids; conflict -external_id_takenfailureenroll_new_userexternal_id_takenfailureexternal_id_takenfailureduplicate_external_id_in_filewhen another CSV row holds the same IDexternal_id_takeninsteadCross-type freed-ID ordering limitation
When an existing course user frees an ext_id and a brand-new user (no Coursemology account) claims it in the same batch, the new user is still rejected with
external_id_taken.invite_new_usersruns beforeadd_existing_usersin the pipeline, so the new invitation is evaluated before the existing course user releases the ID. The same-type freed-ID case (both are existing invitations, or both are course users) works correctly. The spec documents this behavior explicitly rather than asserting it should work - a two-pass fix would add complexity for a narrow edge case.Regression prevention
Service specs cover: nil-to-value upsert for invitations and course users; non-nil-to-different-free-value upsert with
previous_external_idcapture;external_id_takenon new invitations, new enrollments (existing user not yet in course), existing invitation updates, and existing course user updates; conflict anchored by course user ext_id vs invitation ext_id; freed-ID batch claim within the same record type; cross-type freed-ID ordering limitation; unconfirmed-email path sends invitation not direct enrollment;isRetryable: falsepropagated through jbuilder; 6-column CSV uploaded to a non-timeline course raisesCSV::MalformedCSVError(guards the silent-corruption path where timeline strings become external IDs); enrolled user re-uploaded with their own current external ID alongside a conflicting new user passes CSV dedup and reaches the process phase (reason isexternal_id_takennotduplicate_external_id_in_file); enrolled user re-uploaded with their own current external ID and no conflicting new user succeeds with no failures.Model specs cover: blank
external_idon invitation ("") producesnilon the builtCourseUser— guards the.presencecall inUser#build_course_user_from_invitationagainst silent removal (empty string would otherwise pass the partial unique index'sIS NOT NULLguard and cause duplicate-key errors).Frontend specs cover:
InvitationResultExistingTableupdated-row rendering (bold + badge + tooltip);InvitationResultDialog- failed invitations in Failed section not Existing Invitations,failedRowsSubtitleshown only forfailed_to_sendrows with correct count, subtitle absent when only CSV-issue duplicates present, mixed-failure count correctly split between header and caption;InvitationResultFailedTablereason text for all four failure reasons.Manual testing: happy path new batch; nil-to-value upsert with badge; non-nil-to-different upsert with tooltip;
external_id_takenin Failed section; freed-ID same-type batch; existing user no-change; failed-to-send rows in red with caption; CSV-issue rows without caption; mixed failure section showing correct split counts.Backward compatibility: existing invite flows unaffected. The only behavioral change for existing data is that re-inviting a user whose ext_id differs from the CSV value now updates the record instead of rejecting it.
Part 2: Independent improvements
Summary
Five fixes made to the invite flow independent of the external ID feature.
First, invitations with
is_retryable: false(permanent email delivery failures) were previously indistinguishable from normal pending invitations in the Existing Invitations section; they are now routed to a dedicated Failed section, highlighted in red, with a caption identifying exactly which rows could not be sent.Second,
UserInvitationsTablehad two related fixes: it was importinggetManageCourseUserPermissionsfromcourse/users/selectors(readsstate.users) instead of the invitations store (state.invitations), causing the Personalized Timeline column to flicker on SPA navigation; and the column gate was changed fromcanManagePersonalTimestoshowPersonalizedTimelineFeaturesfor correctness (see design decisions below).Third, a conditional "Personalized Timeline" column is added to all three invitation result tables (new invitations/enrollments, existing, and failures). The column appears only when
showPersonalizedTimelineFeaturesis enabled.timeline_algorithmis now serialized on all seven record sections in the result data; the flag is added to the invitation index response, stored in Redux alongsidedefaultTimelineAlgorithm(and added to the initial state of all three bundles that shareManageCourseUsersSharedData: invitations, users, and enrol-requests), read byInvitationResultDialog, and threaded to each table component.Fourth,
build_course_userin the invitation service was ignoring the CSVtimeline_algorithmcolumn for directly-enrolled existing users and always assigning the course default instead. The fix passesuser[:timeline_algorithm]through, consistent withbuild_invitationandbuild_course_user_from_invitation.build_course_user_from_invitationinUseralso had.presenceadded toexternal_idfor consistency withuser_registration_service.Fifth, stale naming is cleaned up throughout the course bundle (service, controller, jbuilder, types, components, specs; instance bundle unchanged):
InvitationResultUsersTable→InvitationResultFailedTable,InvitationResultSkippedTable→InvitationResultExistingTable(with its exported row typeSkippedRow→ExistingRow),DuplicateUserData→FailedInvitationRowData, andduplicate_users/duplicateUsers→failed_users/failedUsers.Design decisions
is_retryable: falseinvitations excluded from the "already invited" summary count - permanently failed invitations are not in the course and will never arrive; counting them as "already invited" misleads the admin into thinking those users are handled.canManagePersonalTimespreserved for editable inputs,showPersonalizedTimelineFeaturesused for read-only display - these two flags are related but distinct.canManagePersonalTimesis the compoundshow_personalized_timeline_features? && can?(:manage_personal_times)— it requires both the course feature to be on and the user to hold the manage permission. The invite forms and CSV upload (unchanged from master) correctly gate oncanManagePersonalTimes: showing an editable timeline field only makes sense when the user can actually set it. The live invitations table (UserInvitationsTable) and the result tables are read-only display — the column should be visible to anyone who can see the page whenever the course feature is on, regardless of whether they hold the manage permission.UserInvitationsTablepreviously usedcanManagePersonalTimes(same as the forms); this PR corrects it toshowPersonalizedTimelineFeaturesto match the result tables. The result tables are new and useshowPersonalizedTimelineFeaturesfrom the start.Regression prevention
Service specs cover:
build_course_userassigns the CSVtimeline_algorithmto the enrolledCourseUser, not the course default (regression guard for the ignored-column bug).Frontend specs cover:
InvitationsIndex- Personalized Timeline column appears whenshowPersonalizedTimelineFeaturesis true in the API response (seeded intostate.invitations) and is absent when false, withstate.usersunpopulated in both cases (regression guard for the wrong-store import and the gate change fromcanManagePersonalTimestoshowPersonalizedTimelineFeatures).showPersonalizedTimelineFeaturesis true, column absent when false, dash rendered whentimelineAlgorithmis undefined - for each ofInvitationResultPrimaryTable,InvitationResultExistingTable, andInvitationResultFailedTable.InvitationResultDialogintegration tests verify the flag is read from the Redux store and threads to tables (column visible withshowPersonalizedTimelineFeatures: truein store, hidden with default store).Manual testing:
is_retryable: falseinvitations appear in Failed section highlighted in red; Personalized Timeline column visible on both SPA navigation and hard refresh when personalized timelines enabled, absent in both when disabled.Backward compatibility: invite forms and CSV upload gate unchanged (
canManagePersonalTimes). Result dialog unchanged for courses with Personalized Timelines disabled.Current Result Display
Happy Path
All Failure Reasons Shown
Timelines Shown In Table If Enabled
Error Flagged if CSV with >5 Columns Used with Timelines Off